Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IBX-6017: Implemented loadVersionInfoListByContentInfo PAPI method #375

Merged
merged 6 commits into from
Jul 31, 2023
Merged

IBX-6017: Implemented loadVersionInfoListByContentInfo PAPI method #375

merged 6 commits into from
Jul 31, 2023

Conversation

barw4
Copy link
Member

@barw4 barw4 commented Jun 28, 2023 •
edited
Loading

Question Answer
JIRA issue IBX-6017
Type feature
Target Ibexa version v3.3
BC breaks no

The newly implemented loadVersionInfoListByContentInfo allows loading latest VersionInfo of given Contents in batch.

Related admin-ui PR: ezsystems/ezplatform-admin-ui#2104

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/engineering-team).

@barw4 barw4 added Feature New feature or request Work in progress labels Jun 28, 2023
@barw4 barw4 self-assigned this Jun 28, 2023
@barw4 barw4 mentioned this pull request Jun 28, 2023
Merged
2 tasks
@barw4 barw4 requested a review from a team June 29, 2023 15:07
alongosz
alongosz requested changes Jul 15, 2023
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this targeted to 3.3 LTS if CS issue is for 4.5.0? This adds unnecessary maintenance costs for QA, cross merge-up, and maintenance. (resolved)

Remarks:

eZ/Publish/API/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Helper/TranslationHelper.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/Gateway/DoctrineDatabaseTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/SPI/Persistence/Content/Handler.php Show resolved Hide resolved
eZ/Publish/Core/Persistence/Cache/ContentHandler.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Content/Gateway.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Content/Handler.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Persistence/Legacy/Tests/Content/ContentHandlerTest.php Outdated Show resolved Hide resolved
@adamwojs
Copy link
Member

@barw4 Could you please apply feedback from @alongosz ?

@barw4
Copy link
Member Author

barw4 commented Jul 17, 2023 •
edited
Loading

Why is this targeted to 3.3 LTS if CS issue is for 4.5.0? This adds unnecessary maintenance costs for QA, cross merge-up, and maintenance. The review anyway:

@alongosz we usually validate whether the same problem occurs in v3.3 if it occurs in v4.5 (and in most cases it does). I believe plenty of installations will benefit from it on v3.3 as it's still LTS even if no one reported it. I can scrap this PR if you wish but I see nothing wrong in improving performance for the active LTS version.

@Steveb-p
Copy link
Contributor

Why is this targeted to 3.3 LTS if CS issue is for 4.5.0? This adds unnecessary maintenance costs for QA, cross merge-up, and maintenance. The review anyway:

@alongosz we usually validate whether the same problem occurs in v3.3 if it occurs in v4.5 (and in most cases it does). I believe plenty of installations will benefit from it on v3.3 as it's still LTS even if no one reported it. I can scrap this PR if you wish but I see nothing wrong in improving performance for the active LTS version.

In theory 3.3 should not receive new functionality. This is done both to incentivize upgrading to a higher version and limit costs of maintenance, as Andrew pointed out.

However, given how we've dealt with it in the past, this performance fix can be accepted, as most of the work is done and I assume you will take care of merging it up later. Ultimately the decision is made by weighting our maintenance costs against benefits.

@alongosz
Copy link
Member

alongosz commented Jul 17, 2023 •
edited
Loading

Why is this targeted to 3.3 LTS if CS issue is for 4.5.0? This adds unnecessary maintenance costs for QA, cross merge-up, and maintenance. The review anyway:

@alongosz we usually validate whether the same problem occurs in v3.3 if it occurs in v4.5 (and in most cases it does). I believe plenty of installations will benefit from it on v3.3 as it's still LTS even if no one reported it. I can scrap this PR if you wish but I see nothing wrong in improving performance for the active LTS version.

if this was an update of internal business logic, that would be just a bug fix and no-brainer. But here you're introducing new API. It always impacts QA, maintenance, and Customers' upgrade process. For that reason I try to avoid packing features into old LTS, unless it's related to CS Customer version. It's fine if you believe it's needed, but in that case we need green light from either @adamwojs or @webhdx so they accept the risk and costs.
Update: resolved.

@alongosz
Copy link
Member

@barw4 after brief discussion with @webhdx we can indeed add this to 3.3 given the amount of work you've put into this and benefits for LTS 💪
Please proceed with the remaining review remarks: #375 (review). Feel free to reach out to me in case of questions :)

@barw4
Copy link
Member Author

barw4 commented Jul 18, 2023

@alongosz thank you for the heads-up, on it

@barw4 barw4 requested review from alongosz and a team July 18, 2023 16:13
@barw4
Copy link
Member Author

barw4 commented Jul 18, 2023

@alongosz almost all your remarks are applied, pinging for re-review

alongosz
alongosz reviewed Jul 18, 2023
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied to the last issue here: #375 (comment)

@barw4 barw4 requested review from alongosz and a team July 19, 2023 08:57
@sonarcloud
Copy link

sonarcloud bot commented Jul 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alongosz alongosz requested a review from a team July 19, 2023 10:09
@konradoboza konradoboza requested a review from a team July 19, 2023 10:15
@micszo micszo self-assigned this Jul 24, 2023
micszo
micszo approved these changes Jul 31, 2023
View reviewed changes
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with ca. 5000k landing pages with 100+ fields. Used tree built with folders with depth 3 (landing pages on 3rd level). One parent had 2, 5, 10, 50, 100 and 500 items.

Tried extreme Content tree config with load_more_limit 500 and children_load_max_limit 5000.
With the fix observed a gain in time from 18-19 sec to ca. 11 sec.

With default Content tree config (load_more_limit 30 and children_load_max_limit 200) the impact was not as big but also clear.
With the fixed observed a gain in time from 11-12 sec to 8-9 sec.

QA Approved on Ibexa Experience 3.3.34-dev.

@micszo micszo removed their assignment Jul 31, 2023
@alongosz alongosz merged commit 002ffa4 into 1.3 Jul 31, 2023
26 checks passed
@alongosz alongosz deleted the ibx-6017-implement-loadVersionInfoList branch July 31, 2023 13:09
@alongosz alongosz mentioned this pull request Aug 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kisztof kisztof kisztof approved these changes

@alongosz alongosz alongosz approved these changes

@micszo micszo micszo approved these changes

@konradoboza konradoboza konradoboza approved these changes

Assignees

@adamwojs adamwojs

@kisztof kisztof

@mikadamczyk mikadamczyk

@Steveb-p Steveb-p

@alongosz alongosz

@ciastektk ciastektk

@barw4 barw4

Labels
Feature New feature or request QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@barw4 @adamwojs @Steveb-p @alongosz @kisztof @micszo @konradoboza @mikadamczyk @ciastektk